Skip to content

fix(cli): stop the hard-coded heap cap from overriding the operator - #3368

Open
ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/cli-heap-cap-3365
Open

fix(cli): stop the hard-coded heap cap from overriding the operator#3368
ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/cli-heap-cap-3365

Conversation

@ntdatt812

Copy link
Copy Markdown
Contributor

Closes #3365. Also the ask in #1982.

cli/cli.js:615 spawns the next-server child with a fixed heap cap:

spawn(RUNTIME, ["--dns-result-order=ipv4first", "--max-old-space-size=6144", serverPath], )

Why that cannot be worked around from outside

Node reads NODE_OPTIONS before command-line flags and lets the command line win. So an operator who sets a lower cap gets it silently discarded. Measured, not assumed:

$ NODE_OPTIONS="--max-old-space-size=256" node --max-old-space-size=4096 -e "…heap_size_limit…"
4288 MB          # the flag wins
$ NODE_OPTIONS="--max-old-space-size=256" node -e "…heap_size_limit…"
448 MB           # NODE_OPTIONS alone works fine

That is the whole bug. On a host with a cgroup limit — systemd MemoryMax, docker --memory, k8s — the child runs believing it has 6 GB. GC thresholds sit near 6 GB, so a 485 MB heap looks like 8% utilisation and collection never becomes urgent; RSS climbs to the cgroup ceiling and the kernel OOM-kills next-server instead. In-flight streaming responses die with the process, the supervisor restarts it, and the cycle repeats — which matches the reporter's 64 restart segments all peaking in the same 478–485 MB band.

What changes

The default is unchanged at 6144 for the desktop case it was raised for. It just stops overriding the operator:

configuration flags passed to the child
nothing set --max-old-space-size=6144
NINEROUTER_MAX_OLD_SPACE_SIZE=384 --max-old-space-size=384
NINEROUTER_MAX_OLD_SPACE_SIZE=0 (none — node sizes the heap itself)
NODE_OPTIONS=--max-old-space-size=384 (none — NODE_OPTIONS is left to apply)
NINEROUTER_MAX_OLD_SPACE_SIZE=abc --max-old-space-size=6144 + a warning

Both mechanisms are covered because they serve different users: NODE_OPTIONS is what a systemd unit or docker run -e already uses, while the dedicated variable is reachable for someone running the tray build or npx 9router who has no convenient place to set node flags.

A junk value keeps the default and warns rather than silently leaving the heap uncapped — losing a safety cap to a typo is the wrong direction to fail in.

Verified end to end against a real child process, not just the resolver:

default                                ["--max-old-space-size=6144"]  => heap limit 6336 MB
NINEROUTER_MAX_OLD_SPACE_SIZE=384      ["--max-old-space-size=384"]   => heap limit  576 MB
NODE_OPTIONS=--max-old-space-size=384  []                             => heap limit  576 MB

The resolver lives in cli/hooks/nodeFlags.js next to sqliteRuntime.js/trayRuntime.js, which is both the existing convention for CLI runtime helpers and what makes it testable — cli.js itself runs on require. hooks/ is already in the package's files, so it ships.

What I did not do

The issue's first suggestion is to derive the default from available RAM. I left it out on purpose: os.totalmem() reports the host's memory, not the cgroup limit, so under docker --memory or a k8s limit it would still report the wrong number — it would not help the case that motivates this report, while changing behaviour for every existing install. Reading /sys/fs/cgroup/memory.max would work but is Linux-only and I have no containerised host here to verify it on. Worth doing as a follow-up, with the caveat that an explicit setting must still win.

The third suggestion — auditing where request/response bodies are retained (#1982, #2472) — is a separate piece of work. As the reporter says, a correct cap lets GC reclaim; it does not remove whatever is retaining.

Verification

8 tests in unit/cli-heap-flags-3365.test.js: the 6144 default is preserved when nothing is set; an explicit cap is honoured (including surrounding whitespace); 0 emits no flag; a --max-old-space-size in NODE_OPTIONS suppresses ours, including the underscore spelling node also accepts and when mixed with other flags; an unrelated NODE_OPTIONS does not; a lookalike flag (--max-old-space-size-hint) does not false-positive; the dedicated variable beats NODE_OPTIONS; junk falls back to the default and warns; an empty/whitespace value counts as unset and warns about nothing.

Mutation-checked — deleting the NODE_OPTIONS passthrough fails exactly the "stands aside when NODE_OPTIONS already caps the heap" test.

node --check cli/cli.js passes. npx eslint reports no issues on the changed files.

Full suite (npx vitest run unit translator): 1808 passed / 95 failed. Two runs differed from master's failing set only in the network-dependent unit/xai-oauth-service.test.js timeouts (and, in one run, two known order-dependent DB tests) — these flip between runs on an unmodified tree too. Nothing under tests/ imports cli/ except the new test, so this change cannot reach them.

The next-server child is spawned with --max-old-space-size=6144 on the command
line. Node reads NODE_OPTIONS first and lets command-line flags win, so that
value cannot be lowered from outside: on a host with a cgroup limit (systemd
MemoryMax, docker --memory, k8s) the child runs believing it has 6 GB of heap.
GC never feels the limit, RSS climbs to the ceiling, and the kernel OOM-kills
next-server — taking in-flight streaming responses with it. The supervisor
restarts and it happens again.

The default is unchanged for the desktop case it was raised for. It now steps
aside once the operator has said what they want:

- NINEROUTER_MAX_OLD_SPACE_SIZE=384 caps the heap at 384 MB; 0 passes no flag
  at all and lets node size the heap from the memory it can see.
- A --max-old-space-size already present in NODE_OPTIONS is respected rather
  than overridden, so the standard mechanism finally works.

A junk value keeps the default and warns, rather than quietly leaving the heap
uncapped — losing a safety cap to a typo is the wrong failure direction.

Deriving the default from available memory is deliberately not done here:
os.totalmem() reports the host's RAM, not the cgroup limit, so it would not
help the containerised case that motivates this and would change behaviour
everywhere else.

Reported in decolua#3365, and previously in decolua#1982.
afandiaziz added a commit to afandiaziz/9router that referenced this pull request Aug 20, 2026
afandiaziz added a commit to afandiaziz/9router that referenced this pull request Aug 20, 2026
…/security/providers

Verified via trial-merge + per-PR tests (84 pass/0 fail), OAuth baseline
identical, providers baseline additive-only (+reasonix/ovh/joycode/openmodel).

PRs: decolua#3411 decolua#3370 decolua#3369 decolua#3368 decolua#3393 decolua#3366 decolua#3395 decolua#3382 decolua#3359 decolua#3408 decolua#3357
     decolua#3379 decolua#3380 decolua#3381 decolua#3396 decolua#3338

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Hard-coded --max-old-space-size=6144 in cli.js makes cgroup/low-memory deployments OOM-loop

1 participant